Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering #133610

camelid · 2024-11-29T06:08:47Z

Fixes #128176.
This accomplishes one of the followup items from #131081.

These operations are much more about lowering the HIR than about
Consts themselves. They fit better in hir_ty_lowering with
lower_const_arg (formerly Const::from_const_arg) and the rest.

To accomplish this, const_evaluatable_predicates_of had to be changed
to not use from_anon_const anymore. Instead of visiting the HIR and
lowering anon consts on the fly, it now visits the rustc_middle::ty
data structures instead and directly looks for UnevaluatedConsts. This
approach was proposed in:
#131081 (comment)

r? @BoxyUwU

rustbot · 2024-11-29T06:09:00Z

HIR ty lowering was modified

cc @fmease

camelid · 2024-11-29T06:12:52Z

compiler/rustc_hir_analysis/src/collect/predicates_of.rs

-    if let Some(generics) = node.generics() {
-        debug!("const_evaluatable_predicates_of({:?}): visit_generics", def_id);
-        collector.visit_generics(generics);


I couldn't really think of anything that would need this, nor what the rustc_middle::ty-equivalent would be. All the tests passed though.

yeah its a bit weird, hir::Generics is some amalgamation of generics_of, and some predicates_of (inline written bounds, e.g. <T: Trait>). We don't really care about generics_of since we don't want to add const evaluatable bounds for const parameter defaults, and any const arguments in types of const parmeters (e.g. <const N: Bar<{ 10 }>>) must be fully concrete so there's no need to add const evaluatable bounds for them.

So yeah 👍 to removing this

camelid · 2024-11-29T06:40:06Z

Curious if this improves performance since we no longer re-lower const args.

@bors try @rust-timer queue

Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering Fixes rust-lang#128176. This accomplishes one of the followup items from rust-lang#131081. These operations are much more about lowering the HIR than about `Const`s themselves. They fit better in hir_ty_lowering with `lower_const_arg` (formerly `Const::from_const_arg`) and the rest. To accomplish this, `const_evaluatable_predicates_of` had to be changed to not use `from_anon_const` anymore. Instead of visiting the HIR and lowering anon consts on the fly, it now visits the `rustc_middle::ty` data structures instead and directly looks for `UnevaluatedConst`s. This approach was proposed in: rust-lang#131081 (comment) r? `@BoxyUwU`

bors · 2024-11-29T06:41:17Z

⌛ Trying commit 352a6c8 with merge 16870bf...

bors · 2024-11-29T08:26:27Z

☀️ Try build successful - checks-actions
Build commit: 16870bf (16870bffd86d10e5ea2f720601410e5fdeeed00a)

rust-timer · 2024-11-29T10:51:31Z

Finished benchmarking commit (16870bf): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.9%	[1.1%, 4.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-1.4%, -1.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[-1.4%, 4.6%]	4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 773.667s -> 772.898s (-0.10%)
Artifact size: 331.97 MiB -> 331.97 MiB (-0.00%)

BoxyUwU

very cool! :3 small review + fixing the intra doc link that's now broken then should be good to go

compiler/rustc_hir_analysis/src/collect/predicates_of.rs

BoxyUwU · 2024-11-29T15:27:05Z

tests/ui/const-generics/issues/cg-in-dyn-issue-128176.rs

-//@ known-bug: rust-lang/rust#128176
+//@ check-pass
+
+// Regression test for #128176.


Suggested change

// Regression test for #128176.

// Regression test for #128176. Previously we would call `type_of` on the `1` anon const before the anon const had been lowered and had the `type_of` fed with a result.

BoxyUwU · 2024-11-29T15:29:09Z

I wouldn't have really expected any perf effects since most of this PR only has an effect under generic_const_exprs so good that it's neutral :3

BoxyUwU · 2024-11-30T10:56:00Z

@bors r+

don't need to block this PR on figuring out exactly why that test stopped ICEing since its gce only

bors · 2024-11-30T10:56:03Z

📌 Commit 5c37ec1 has been approved by BoxyUwU

It is now in the queue for this repository.

bors · 2024-11-30T10:56:04Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

BoxyUwU · 2024-12-02T12:52:08Z

@bors rollup=maybe

bors · 2024-12-02T15:52:20Z

☔ The latest upstream changes (presumably #133728) made this pull request unmergeable. Please resolve the merge conflicts.

These operations are much more about lowering the HIR than about `Const`s themselves. They fit better in hir_ty_lowering with `lower_const_arg` (formerly `Const::from_const_arg`) and the rest. To accomplish this, `const_evaluatable_predicates_of` had to be changed to not use `from_anon_const` anymore. Instead of visiting the HIR and lowering anon consts on the fly, it now visits the `rustc_middle::ty` data structures instead and directly looks for `UnevaluatedConst`s. This approach was proposed in: rust-lang#131081 (comment)

camelid · 2024-12-02T21:03:53Z

@bors r=BoxyUwU

bors · 2024-12-02T21:03:55Z

📌 Commit dcf332b has been approved by BoxyUwU

It is now in the queue for this repository.

…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#131713 (Stabilize `const_maybe_uninit_write`) - rust-lang#133535 (show forbidden_lint_groups in future-compat reports) - rust-lang#133610 (Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering) - rust-lang#133701 (Use c"lit" for CStrings without unwrap) - rust-lang#133704 (fix ICE when promoted has layout size overflow) - rust-lang#133705 (add "profiler" and "optimized-compiler-builtins" option coverage for ci-rustc) - rust-lang#133710 (Reducing `target_feature` check-cfg merge conflicts) - rust-lang#133732 (Fix `-Zdump-mir-dataflow`) - rust-lang#133746 (Change `AttrArgs::Eq` to a struct variant) - rust-lang#133763 (Fix `f16::midpoint` const feature gate) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#133610 - camelid:move-from_anon_const, r=BoxyUwU Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering Fixes rust-lang#128176. This accomplishes one of the followup items from rust-lang#131081. These operations are much more about lowering the HIR than about `Const`s themselves. They fit better in hir_ty_lowering with `lower_const_arg` (formerly `Const::from_const_arg`) and the rest. To accomplish this, `const_evaluatable_predicates_of` had to be changed to not use `from_anon_const` anymore. Instead of visiting the HIR and lowering anon consts on the fly, it now visits the `rustc_middle::ty` data structures instead and directly looks for `UnevaluatedConst`s. This approach was proposed in: rust-lang#131081 (comment) r? `@BoxyUwU`

rustbot assigned BoxyUwU Nov 29, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 29, 2024

camelid added the A-const-generics Area: const generics (parameters and arguments) label Nov 29, 2024

camelid commented Nov 29, 2024

View reviewed changes

camelid force-pushed the move-from_anon_const branch from d660c44 to 0c762b6 Compare November 29, 2024 06:27

camelid mentioned this pull request Nov 29, 2024

ICE: expected const for .. but found Type(usize) when instantiating args=[usize] #121429

Open

This comment has been minimized.

Sign in to view

camelid force-pushed the move-from_anon_const branch from 0c762b6 to 352a6c8 Compare November 29, 2024 06:39

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 29, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 29, 2024

BoxyUwU reviewed Nov 29, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2024

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 2, 2024

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 2, 2024

BoxyUwU force-pushed the move-from_anon_const branch from 5c37ec1 to 4dd0b08 Compare December 2, 2024 17:43

This comment has been minimized.

Sign in to view

camelid added 2 commits December 2, 2024 19:34

Fix broken intra-doc link

dcf332b

BoxyUwU force-pushed the move-from_anon_const branch from 4dd0b08 to dcf332b Compare December 2, 2024 19:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2024

GuillaumeGomez mentioned this pull request Dec 2, 2024

Rollup of 10 pull requests #133770

Merged

bors merged commit 6f0d15a into rust-lang:master Dec 3, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 3, 2024

camelid deleted the move-from_anon_const branch December 3, 2024 05:44

matthiaskrgr mentioned this pull request Dec 3, 2024

ICE: DefId::expect_local DefId isn't local #133808

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering #133610

Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering #133610

camelid commented Nov 29, 2024 •

edited

Loading

rustbot commented Nov 29, 2024

camelid Nov 29, 2024

BoxyUwU Nov 29, 2024

This comment has been minimized.

camelid commented Nov 29, 2024

This comment has been minimized.

bors commented Nov 29, 2024

This comment has been minimized.

bors commented Nov 29, 2024

This comment has been minimized.

rust-timer commented Nov 29, 2024

BoxyUwU left a comment

BoxyUwU Nov 29, 2024 •

edited

Loading

BoxyUwU commented Nov 29, 2024

BoxyUwU commented Nov 30, 2024

bors commented Nov 30, 2024

bors commented Nov 30, 2024

BoxyUwU commented Dec 2, 2024

bors commented Dec 2, 2024

This comment has been minimized.

camelid commented Dec 2, 2024

bors commented Dec 2, 2024

	// Regression test for #128176.
	// Regression test for #128176. Previously we would call `type_of` on the `1` anon const before the anon const had been lowered and had the `type_of` fed with a result.

Move Const::{from_anon_const,try_from_lit} to hir_ty_lowering #133610

Move Const::{from_anon_const,try_from_lit} to hir_ty_lowering #133610

Conversation

camelid commented Nov 29, 2024 • edited Loading

rustbot commented Nov 29, 2024

camelid Nov 29, 2024

Choose a reason for hiding this comment

BoxyUwU Nov 29, 2024

Choose a reason for hiding this comment

This comment has been minimized.

camelid commented Nov 29, 2024

This comment has been minimized.

bors commented Nov 29, 2024

This comment has been minimized.

bors commented Nov 29, 2024

This comment has been minimized.

rust-timer commented Nov 29, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

BoxyUwU left a comment

Choose a reason for hiding this comment

BoxyUwU Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

BoxyUwU commented Nov 29, 2024

BoxyUwU commented Nov 30, 2024

bors commented Nov 30, 2024

bors commented Nov 30, 2024

BoxyUwU commented Dec 2, 2024

bors commented Dec 2, 2024

This comment has been minimized.

camelid commented Dec 2, 2024

bors commented Dec 2, 2024

Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering #133610

Move `Const::{from_anon_const,try_from_lit}` to hir_ty_lowering #133610

camelid commented Nov 29, 2024 •

edited

Loading

BoxyUwU Nov 29, 2024 •

edited

Loading